azurerm_redis_cache - support access_keys_authentication_disabled property #27039

gerrytan · 2024-08-14T12:38:17Z

Community Note

Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

Adds support for access_keys_authentication_disabled property for azurerm_redis_cache

PR Checklist

I have followed the guidelines in our Contributing Documentation.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
For example: “resource_name_here - description of change e.g. adding property new_property_name_here”

Changes to existing Resource / Data Source

I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
I have written new tests for my resource or datasource changes & updated any relevent documentation.
I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
~~(For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.~~

Testing

My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

make acctests SERVICE='redis' TESTARGS='-run=TestAccRedisCache_AccessKeysAuthenticationEnabledDisabled' TESTTIMEOUT='120m' -
output log
TestAccRedisCacheDataSource_standard - output log

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

azurerm_redis_cache - support for access_keys_authentication_disabled property [Support for Microsoft.Cache for Redis API v2024-03-01 #26797]

This is a (please select all that apply):

Bug Fix
New Feature (ie adding a service, resource, or data source)
Enhancement
Breaking Change

Related Issue(s)

Fixes #26797

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

magodo

Thanks for this PR!
I have left some additional mostly minor comments that once addressed this should be good to merge 👍

website/docs/r/redis_cache.html.markdown

website/docs/d/redis_cache.html.markdown

internal/services/redis/validate/access_keys_auth.go

magodo

Some minor comments, otherwise LGTM

internal/services/redis/redis_cache_resource.go

stephybun

Thanks for this PR @gerrytan. Please see the comment left in-line, the property will need to be renamed to conform to our naming guidelines. Once that's done and the logic amended we can take another look through.

internal/services/redis/redis_cache_data_source.go

internal/services/redis/redis_cache_resource.go

magodo · 2024-08-19T11:56:41Z

Thanks @gerrytan, it LGTM now!

…shicorp#26797)

Co-authored-by: magodo <[email protected]>

To comply with Hashicorp contrib guide

Co-authored-by: magodo <[email protected]>

gerrytan · 2024-08-20T04:51:15Z

@stephybun @magodo this PR is now good for review. All feedbacks have been addressed and acctests have been rerun successfully, check the output log on the description.

stephybun

Thanks @gerrytan, other than the two minor comments left in-line this looks good. Once those are resolved this should be ready to go in!

stephybun · 2024-08-20T08:15:55Z

internal/services/redis/redis_cache_data_source.go

+		accessKeyAuthEnabled := true
+		if props.DisableAccessKeyAuthentication != nil {
+			accessKeyAuthEnabled = !(*props.DisableAccessKeyAuthentication)
+		}
+		d.Set("access_keys_authentication_enabled", accessKeyAuthEnabled)


It's a common pattern in the provider to make use of pointer.From when setting properties that are pointers into state. pointer.From will nil check the input and if the pointer is nil will return the zero value for the type that was passed in, so this would be condensed down to:

Suggested change

accessKeyAuthEnabled := true

if props.DisableAccessKeyAuthentication != nil {

accessKeyAuthEnabled = !(*props.DisableAccessKeyAuthentication)

}

d.Set("access_keys_authentication_enabled", accessKeyAuthEnabled)

d.Set("access_keys_authentication_enabled", !pointer.From(props.DisableAccessKeyAuthentication))

@stephybun is right, I definitely missed this out.

Ah nice idea, thx @stephybun . I will alter the suggested code a bit: we want to make the expression defaults to true if we get nil value on DisableAccessKeyAuthentication.

Addressed in 50de57e

@gerrytan that is the outcome of my suggestion above.

If props.DisableAccessKeyAuthentication is nil, pointer.From will return false (the zero value for bools) which will result in true since it's being inverted. The variable initialization and nil check on lines 357-361 are redundant. Can you please apply the suggestion as made?

@stephybun ah I get it now. Fixed in 5341f2f.

stephybun · 2024-08-20T08:17:05Z

internal/services/redis/redis_cache_resource.go

+		accessKeyAuthEnabled := true
+		if props.DisableAccessKeyAuthentication != nil {
+			accessKeyAuthEnabled = !(*props.DisableAccessKeyAuthentication)
+		}
+		d.Set("access_keys_authentication_enabled", accessKeyAuthEnabled)


Suggested change

accessKeyAuthEnabled := true

if props.DisableAccessKeyAuthentication != nil {

accessKeyAuthEnabled = !(*props.DisableAccessKeyAuthentication)

}

d.Set("access_keys_authentication_enabled", accessKeyAuthEnabled)

d.Set("access_keys_authentication_enabled", !pointer.From(props.DisableAccessKeyAuthentication))

Addressed in 50de57e

Fixed in 5341f2f

stephybun

See responses to ongoing discussion in comments

stephybun

Apologies if my initial explanation of the pointer.From function wasn't clear, thanks for making those changes @gerrytan LGTM 🍎

github-actions · 2024-09-22T02:15:28Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

github-actions bot added documentation service/redis size/M labels Aug 14, 2024

gerrytan mentioned this pull request Aug 14, 2024

#26797: disable_access_keys_authentication property for Redis Cache #26964

Closed

14 tasks

github-actions bot added the waiting-response label Aug 14, 2024

gerrytan force-pushed the redis-cache-access-keys-auth-prop branch from 199cea6 to 5201b0a Compare August 14, 2024 13:39

github-actions bot added waiting-response and removed waiting-response labels Aug 14, 2024

gerrytan marked this pull request as ready for review August 14, 2024 23:49

magodo reviewed Aug 16, 2024

View reviewed changes

website/docs/r/redis_cache.html.markdown Outdated Show resolved Hide resolved

website/docs/d/redis_cache.html.markdown Outdated Show resolved Hide resolved

internal/services/redis/validate/access_keys_auth.go Outdated Show resolved Hide resolved

github-actions bot added waiting-response and removed waiting-response labels Aug 16, 2024

magodo reviewed Aug 16, 2024

View reviewed changes

internal/services/redis/redis_cache_resource.go Outdated Show resolved Hide resolved

internal/services/redis/redis_cache_resource.go Outdated Show resolved Hide resolved

github-actions bot added waiting-response and removed waiting-response labels Aug 16, 2024

magodo added the enhancement label Aug 16, 2024

stephybun requested changes Aug 16, 2024

View reviewed changes

internal/services/redis/redis_cache_data_source.go Outdated Show resolved Hide resolved

github-actions bot added waiting-response and removed waiting-response labels Aug 16, 2024

magodo requested changes Aug 18, 2024

View reviewed changes

github-actions bot added waiting-response and removed waiting-response labels Aug 19, 2024

gerrytan and others added 4 commits August 20, 2024 09:29

azurerm_redis_cache: access_keys_authentication_disabled property (ha…

6ebbc0e

…shicorp#26797)

Fix tflint and unused test func arg

9dcb9a3

Update website/docs/d/redis_cache.html.markdown

0055fb8

Co-authored-by: magodo <[email protected]>

Inlined access keys auth validation into CustomizeDiff

bf232ce

gerrytan and others added 8 commits August 20, 2024 09:29

Further improvement on website/docs/r/redis_cache.html.markdown

c71024a

Improved debug log message

bc869cc

Co-authored-by: magodo <[email protected]>

Improved error message

c04488b

Co-authored-by: magodo <[email protected]>

Flip prop into _enabled

983a600

To comply with Hashicorp contrib guide

Fix linting error on redis_cache_resource_test.go

7e4a511

Added nil check on redis_cache_data_source.go

882fecd

Co-authored-by: magodo <[email protected]>

Formatting on redis_cache_data_source.go

b384de3

More nil check and boolean prop flipping on redis_cache_resource.go

a68b456

gerrytan force-pushed the redis-cache-access-keys-auth-prop branch from faa348a to a68b456 Compare August 19, 2024 23:30

gerrytan requested review from magodo and stephybun August 19, 2024 23:37

stephybun requested changes Aug 20, 2024

View reviewed changes

github-actions bot added the waiting-response label Aug 20, 2024

PR feedback: use pointer.From

50de57e

github-actions bot removed the waiting-response label Aug 21, 2024

gerrytan requested a review from stephybun August 21, 2024 00:59

github-actions bot added the waiting-response label Aug 21, 2024

stephybun requested changes Aug 21, 2024

View reviewed changes

Removed redundant nil check

5341f2f

github-actions bot removed the waiting-response label Aug 21, 2024

Merge branch 'main' into redis-cache-access-keys-auth-prop

4d2e758

gerrytan requested a review from stephybun August 21, 2024 08:32

stephybun approved these changes Aug 21, 2024

View reviewed changes

stephybun merged commit ca742ee into hashicorp:main Aug 21, 2024
33 checks passed

github-actions bot added this to the v3.117.0 milestone Aug 21, 2024

stephybun modified the milestones: v3.117.0, v4.0.0 Aug 21, 2024

github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

azurerm_redis_cache - support access_keys_authentication_disabled property #27039

azurerm_redis_cache - support access_keys_authentication_disabled property #27039

gerrytan commented Aug 14, 2024 •

edited

Loading

magodo left a comment

magodo left a comment

stephybun left a comment

magodo commented Aug 19, 2024

gerrytan commented Aug 20, 2024

stephybun left a comment

stephybun Aug 20, 2024

magodo Aug 20, 2024

gerrytan Aug 21, 2024

gerrytan Aug 21, 2024

stephybun Aug 21, 2024

gerrytan Aug 21, 2024

stephybun Aug 20, 2024

gerrytan Aug 21, 2024

stephybun Aug 21, 2024

gerrytan Aug 21, 2024

stephybun left a comment

stephybun left a comment

github-actions bot commented Sep 22, 2024

azurerm_redis_cache - support access_keys_authentication_disabled property #27039

azurerm_redis_cache - support access_keys_authentication_disabled property #27039

Conversation

gerrytan commented Aug 14, 2024 • edited Loading

Community Note

Description

PR Checklist

Changes to existing Resource / Data Source

Testing

Change Log

Related Issue(s)

magodo left a comment

Choose a reason for hiding this comment

magodo left a comment

Choose a reason for hiding this comment

stephybun left a comment

Choose a reason for hiding this comment

magodo commented Aug 19, 2024

gerrytan commented Aug 20, 2024

stephybun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephybun left a comment

Choose a reason for hiding this comment

stephybun left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 22, 2024

gerrytan commented Aug 14, 2024 •

edited

Loading